Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ioc: improve long string detection #47

Merged
merged 2 commits into from
Jun 22, 2023
Merged

Conversation

mdavidsaver
Copy link
Member

@mdavidsaver mdavidsaver commented Jun 8, 2023

Attempt to improve automatic handling of "long strings". At least to handle the $ case.

I had hoped that I could come up with a way to detect the effects of $ or similar server side filters by inspecting the types stored in the dbChannel and dbAddr structs. However, I've almost reached the point of giving up. Between the mangling done in dbChannelCreate(), and in various rset::cvt_dbaddr(), I don't think this can reliably be done without extending dbChannel.

And there are also cases like waveformRecord with FTVL=CHAR, where .VAL$ has never been supported because the underlying field type isn't DBR_STRING.

@anjohnson @coretl fyi.

@mdavidsaver mdavidsaver self-assigned this Jun 8, 2023
@coretl
Copy link
Collaborator

coretl commented Jun 8, 2023

@mdavidsaver thanks, this fixes my immediate problem, reading lsi_record.VAL$ as a string. I tried it out using the wheel generated as an artifact from this PR, and pythonSoftIOC from DiamondLightSource/pythonSoftIOC#132

I'll consider what the best mix of DiamondLightSource/pythonSoftIOC#132 and DiamondLightSource/pythonSoftIOC#133 is to merge when there's a new release of pvxslibs to point at

@AppVeyorBot
Copy link

Build pvxs 1.0.898 completed (commit 324c44af22 by @mdavidsaver)

@mdavidsaver
Copy link
Member Author

... when there's a new release of pvxslibs to point at

Unless I hear cries of horror from @anjohnson this week, I'll merge and release.

@anjohnson
Copy link
Member

I haven’t checked the code since I’m only using an iPad right now, but I believe it’s legal to follow a $ modifier with a subarray […] modifier which wouldn’t work with this.

Is it not possible to support lsi.VAL as a long string directly by looking at the field size? I thought there was a way to do that since copying long strings through a DB link is possible IIRC, maybe look at the lsi softDev support for ideas? Sorry I don’t have time to look myself right now.

@mdavidsaver
Copy link
Member Author

Is it not possible to support lsi.VAL as a long string directly by looking at the field size? ...

Probably, although this does raise the issue of retraining users not to append $, and live with a difference between caget and pvget. Still, perhaps this is the way forward.

I have also found myself considering always mapping DBF_CHAR array to PVA string. DBF_UCHAR would continue to map to uint8_t[] for actual byte arrays. Of course, this would also be a difference with both CA and earlier QSRV...

@coretl
Copy link
Collaborator

coretl commented Jun 9, 2023

Is it not possible to support lsi.VAL as a long string directly by looking at the field size? ...

Probably, although this does raise the issue of retraining users not to append $, and live with a difference between caget and pvget. Still, perhaps this is the way forward.

Personally I would prefer this. For my 3 use cases it would be great if pvget worked without the $:

  • pvget lsi.VAL
  • pvget calc.INPA
  • pvget anyrecord.NAME

I would be happy to move any of my usage of long strings in a waveform record to use lsi/lso records if this was supported.

I have also found myself considering always mapping DBF_CHAR array to PVA string. DBF_UCHAR would continue to map to uint8_t[] for actual byte arrays. Of course, this would also be a difference with both CA and earlier QSRV...

For waveform records? I guess some fast ADCs might produce signed 8-bit data via waveform records, although that's pretty rare.

@coretl
Copy link
Collaborator

coretl commented Jun 9, 2023

I have also found myself considering always mapping DBF_CHAR array to PVA string. DBF_UCHAR would continue to map to uint8_t[] for actual byte arrays. Of course, this would also be a difference with both CA and earlier QSRV...

How about an opt-in for this, like Q:form = String on the waveform record to turn on mapping of DBF_CHAR array to PVA string?

@mdavidsaver mdavidsaver force-pushed the better-long-string-detection branch from f80c194 to 3a8c882 Compare June 9, 2023 23:13
@AppVeyorBot
Copy link

Build pvxs 1.0.900 completed (commit f034043f70 by @mdavidsaver)

@mdavidsaver
Copy link
Member Author

Is it not possible to support lsi.VAL as a long string directly by looking at the field size? ...

Probably, although this does raise the issue of retraining users not to append $, and live with a difference between caget and pvget. Still, perhaps this is the way forward.

Personally I would prefer this. For my 3 use cases it would be great if pvget worked without the $:

I've rewritten around this idea. I think the changes in the test code illustrate things.

Reading test:this:is:a:really:really:long:record:name.NAME would no longer truncate. Similarly for long link strings. The current behavior with $ is preserved.

With this change, access to lsi should "just work" without a $. Behavior with a $ is unchanged (treated as int8_t[]).

For waveform records? I guess some fast ADCs might produce signed 8-bit data via waveform records, although that's pretty rare.

This is a point...

How about an opt-in for this, like Q:form = String on the waveform record ...

I think I have figured out a way to make this work without adding another dbFindRecord() with 3.15 (3.16.1 adds dbInitEntryFromRecord() which O(0)).

record(waveform, "test:long:str:wf") {
field(FTVL, "CHAR")
field(NELM, "128")
info(Q:form, "String")
}

@mdavidsaver
Copy link
Member Author

@anjohnson I've opted for "mangling" the dbChannel in between dbChannelCreate() and dbChannelOpen(). The logic is effectively a copy of what is currently done in dbChannelCreate(). I don't especially like doing this, and worry about compatibility going forward if/when dbChannelCreate() changes. However, the only alternative I see would be parsing and mangling the PV name string prior to dbChannelCreate(), which I like less.

Also, at the moment I'm the "mangling" logic doesn't test ellCount(&chan->filters) since dbChannelCreate() doesn't. However, I'm considering restricting "mangling" to only when no filters are used. Mainly because I don't have a good idea of what to test, and how things might go wrong.

@anjohnson
Copy link
Member

I haven't tried to compare the code, but I have an impression that this is cleaner than the previous version (although I don't remember looking at that too closely). I was hoping we could offer a solution such that users at PVA-only sites won't need to worry about $ or the 40-char string size limit anywhere near as much, and this does seem to offer that.

Could adding an alternate dbChannelCreateLS() routine to Base remove the concern about later modifications to the dbChannel code? It would do something like always exposing DBF_STRING fields to the caller as an array of DBR_CHAR, which I think is what you're effectively doing. There may be a problem with that, but I can't see it right now.

My one concern is that I assume adding info(Q:form, "String") applies to all fields of the record. Can it be configured any more finely than that? An aSub record might have one field that holds a long string and another with an array of int8 values. This seems likely to be rare, but not impossible (so there would probably be a tech-talk question about supporting it 2 weeks after you release this change!).

@mdavidsaver
Copy link
Member Author

mdavidsaver commented Jun 13, 2023

Could adding an alternate dbChannelCreateLS() routine to ... do something like always exposing DBF_STRING

How about adding a flags argument? With one flag requesting automatic promotion of DBF_STRING to DBF_CHAR[].

It will also be necessary to give some indication that this promotion has happened. This is really the missing piece in dbChannelCreate() now which would remove most of the ambiguity of intent wrt. long string vs. array of signed bytes.

I was thinking to add a flag like dbChannel::longString, although with a new function there are other possibilities to get this information back to the caller.

My one concern is that I assume adding info(Q:form, "String") applies to all fields of the record.

Yup. I didn't think of this back when Q:form was added. This tag was used by pva2pva as well. I could limit this hint to VAL.

Of course aSub is an extra special case, which I'm inclined to ignore for the present.

@AppVeyorBot
Copy link

Build pvxs 1.0.902 completed (commit efaff700b4 by @mdavidsaver)

@mdavidsaver
Copy link
Member Author

My one concern is that I assume adding info(Q:form, "String") applies to all fields of the record.

Yup. I didn't think of this back when Q:form was added. This tag was used by pva2pva as well. I could limit this hint to VAL.

I have updated things such that the Q:form hint only applies when dbIsValueField().

@mdavidsaver mdavidsaver added the enhancement New feature or request label Jun 14, 2023
@AppVeyorBot
Copy link

Build pvxs 1.0.906 completed (commit cd06817c7e by @mdavidsaver)

Partially replicate the '$' handling logic of dbChannelCreate()
in the case where no '$' was provided.
@mdavidsaver
Copy link
Member Author

@coretl If you have time, can you take another look at this? Despite 1.2.1 only being 6 days old, after working through clang-tidy results, I am thinking about 1.2.2. If it is ready, I'd like to include this PR as well.

@AppVeyorBot
Copy link

Build pvxs 1.0.909 failed (commit daf36adbac by @mdavidsaver)

@AppVeyorBot
Copy link

Build pvxs 1.0.909 completed (commit daf36adbac by @mdavidsaver)

@coretl
Copy link
Collaborator

coretl commented Jun 20, 2023

I've installed the python3.10 linux wheel from https://github.com/mdavidsaver/pvxs/actions/runs/5304865866 and it seems to work fine for me, no $ characters needed for my use cases. Thanks, looking forward to 1.2.2!

@mdavidsaver mdavidsaver merged commit a02b6e9 into master Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants